-
Notifications
You must be signed in to change notification settings - Fork 34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Allow to use secret with key-value pairs #202
Conversation
1926616
to
6f51719
Compare
Thanks for your PR, Is it ready for review? |
I'll try to check this in actual environment still today that it works as expected. So trying to get it ready for review today. |
Some issues that needs to be checked still with Golang, so might take a while to find time to focus on those. |
af2d2f8
to
aa114d3
Compare
aa114d3
to
3d58f51
Compare
@wchaws Now Lambda seemed to work as should, so I think this is ready to be reviewed. Also build works in Docker. |
@wchaws Do you have had time to check this? |
@wchaws how about this? |
@Hi-Fi Sorry for the late reply. I was so busy for the last few months and didn't have time to review your PR until now. |
If remember correctly, I checked unit tests for that but it would require some refactoring to mock those secrets manager calls. But I can try to recheck those. I think reflection is needed, and needs to be done also for S3 as currently main tests are skipped also due to that. |
547335b
to
5739a25
Compare
@wchaws I added testing for secretsmanager now. I didn't add mocking to existing tests that were skipped, even same approach could be used for those to mock out e.g. s3. |
BREAKING CHANGE: API to define credentials changed
5739a25
to
62d744f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Hi @Hi-Fi, First, thanks for your contribution. But since this PR is blocking the sub-sequential release, I think I have to revert this commit. Besides that, I've been getting security alerts for quite some time now. Since our company mandates that these security vulnerabilities be solved, I think there are some issues with the current design. I will re-propose a new design to avoid receiving a lot of security vulnerabilities from go dependencies. The main idea is to download the skopeo cli tool directly instead of building go projects from scratch. I hope you can understand! |
BREAKING CHANGE: API to define credentials changed
Fixes #179